参考書籍一覧ページに必読マークを表示する #5674

hikarook94 · 2022-10-25T11:14:56Z

Issue

参考書籍一覧で、書籍が何かしらのプラクティスで必読だったら、必読マークを表示する。 #5571

概要

/books書籍一覧ページで書籍が何かしらのプラクティスで必読に指定されている場合は必読マークを表示するように変更しました。

変更確認方法

ブランチfeature/display-must-read-mark-in-booksをローカルに取り込む
サーバーを起動する
mentormentaroでログインする
http://localhost:3000/booksにアクセスする
『ゼロからわかるRuby超入門』に必読マークが付いていることを確認する
http://localhost:3000/practices/199563205/editにアクセスする
書籍を選択ボタンを押す
『はじめて学ぶソフトウェアのテスト技法』を選択、必読をONにして、更新するボタンを押す
http://localhost:3000/booksに再びアクセスする
『はじめて学ぶソフトウェアのテスト技法』に必読マークが付いていることを確認する

変更前

変更後

hikarook94 · 2022-10-27T07:25:14Z

@yamatatsu10969
お疲れ様です！
ご都合がよければこちらのPRのレビューをお願いしたいのですがいかがでしょうか？

yamatatsu10969 · 2022-10-27T23:15:46Z

@hikarook94
ご依頼いただきありがとうございます！
レビューさせていただきます🚀

yamatatsu10969

@hikarook94
レビューさせていただきました！

ローカルでの動作問題なかったです🙌

数点、質問や提案をさせていただいているのでご確認ください🙏

yamatatsu10969 · 2022-10-27T23:21:29Z

app/helpers/books_helper.rb

+# frozen_string_literal: true
+
+module BooksHelper
+  def must_read_for_any_practice?(book)


must_read_for_any_practice? だと どんなプラクティスでも必読？ と理解してしまいそうなので、シンプルに must_read? でも良さそうだと思いました！

たしかにメソッド名わかりづらいですね...😓
本とプラクティスの関連を表すpractices_booksテーブルがmust_read属性を持っていてpractices_books.must_read?と名前が重複してしまうかなという思いもあり、少し具体的な名前にしたいと考えました。
いずれかのプラクティスで必読？みたいなメソッド名が良いかと思うのですがいかがでしょうか？

いずれかのプラクティスで必読？みたいなメソッド名が良いかと思うのですがいかがでしょうか？

そちらでもわかりやすいので良いと思います〜！

ありがとうございます〜！
any + 複数形 + ?で一つでも存在しているか？となるようにメソッド名を修正しました！

yamatatsu10969 · 2022-10-27T23:31:59Z

app/helpers/books_helper.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+module BooksHelper


BooksHelper を作ることで view でのロジックを減らして読みやすくするんですね〜 👍

yamatatsu10969 · 2022-10-27T23:48:02Z

app/controllers/api/books_controller.rb

@@ -2,6 +2,6 @@

 class API::BooksController < API::BaseController
  def index
-    @books = Book.with_attached_cover.includes(:practices)
+    @books = Book.with_attached_cover.includes(%i[practices practices_books])


自分が includes に対する理解が浅いので質問です！
practices_books を追加した理由を教えていただけると助かります🙏
(ローカルで practices_books を削除して実行しても特に動作は変わらなかったため。)

includes の使い方を調査したところN+1問題を解決するために使用されるようですね🧐

https://pikawaka.com/rails/includes

上記の記事を読んだ自分の理解は以下になります。

practices_books を includes に指定することで事前に practices_books を取得できるので、BooksHelper の book.practices_books の時にクエリを発行せずに済む。

この理解で合っていますでしょうか？

質問ありがとうございます！
僕もその理解で追加したのですが、質問をいただいて改めて調べたところBook.with_attached_cover.includes(:practices)だけでもpractices_booksを取得するクエリが発行されているようなのであまり意味はなかったかもしれません...削除しようかなと思います。

そうなんですね！実行までありがとうございます🙏
削除でも大丈夫そうですね！

hikarook94 · 2022-10-29T14:22:25Z

@yamatatsu10969
レビューありがとうございました！
一部コメントに返信しています。ご確認よろしくお願いいたします！

yamatatsu10969 · 2022-10-30T13:12:41Z

@hikarook94
コメントさせていただきました！
コメントいただいた方針で大丈夫だと思います🙆‍♂️

hikarook94 · 2022-10-31T03:19:07Z

@yamatatsu10969
お疲れ様です！
たびたびすみませんが修正しました！ご確認よろしくお願いいたします🙇‍♂️

yamatatsu10969

修正ありがとうございました！
LGTMです🚀

hikarook94 · 2022-11-01T01:47:03Z

@yamatatsu10969
レビューありがとうございました！

@komagata
メンバーの方にApproveいただいたのでレビューをお願いいたします🙏

komagata · 2022-11-07T00:47:23Z

app/helpers/books_helper.rb

+# frozen_string_literal: true
+
+module BooksHelper
+  def must_read_for_any_practices?(book)


Viewでしか使わないのであればDecoratorにするのがいいかもです〜

ありがとうございます！Decoratorにしてみました！
ご確認よろしくお願いします🙏

komagata · 2022-11-08T02:06:12Z

app/decorators/book_decorator.rb

+# frozen_string_literal: true
+
+module BookDecorator
+  def must_read_for_any_practices?


Decoratorに独自のメソッドを追加した場合は対応するテストが欲しい感じです〜

テストを追加しました！ご確認よろしくお願いいたします🙏

komagata · 2022-11-11T06:27:02Z

test/decorators/book_decorator_test.rb

+    assert_equal true, @book1.must_read_for_any_practices?
+    assert_equal false, @book2.must_read_for_any_practices?


Suggested change

assert_equal true, @book1.must_read_for_any_practices?

assert_equal false, @book2.must_read_for_any_practices?

assert @book1.must_read_for_any_practices?

assert_not @book2.must_read_for_any_practices?

trueかどうかのテストはassertがいいと思います〜

修正しました！ご確認よろしくお願いいたします！

komagata

確認させて頂きました。OKです〜🙆‍♂️

machida assigned hikarook94 Oct 26, 2022

hikarook94 force-pushed the feature/display-must-read-mark-in-books branch from 1f5c8f9 to 4793853 Compare October 26, 2022 07:02

hikarook94 changed the title ~~Feature/display must read mark in books~~ 参考書籍一覧ページに必読マークを表示する Oct 27, 2022

hikarook94 requested a review from yamatatsu10969 October 27, 2022 07:22

hikarook94 marked this pull request as ready for review October 27, 2022 07:25

yamatatsu10969 reviewed Oct 27, 2022

View reviewed changes

hikarook94 force-pushed the feature/display-must-read-mark-in-books branch from 4793853 to 2ba62ce Compare October 31, 2022 01:54

yamatatsu10969 approved these changes Oct 31, 2022

View reviewed changes

hikarook94 requested a review from komagata November 1, 2022 01:47

komagata reviewed Nov 7, 2022

View reviewed changes

hikarook94 force-pushed the feature/display-must-read-mark-in-books branch from 2ba62ce to d870b3b Compare November 7, 2022 08:08

komagata reviewed Nov 8, 2022

View reviewed changes

hikarook94 force-pushed the feature/display-must-read-mark-in-books branch 2 times, most recently from 818ed67 to 086db78 Compare November 8, 2022 05:53

komagata reviewed Nov 11, 2022

View reviewed changes

hikarook94 added 7 commits November 11, 2022 15:56

必読か判定してマークを出すよう変更

88b8b55

プラクティス必読の本であるかのメンバーを追加

9b42c8a

メソッド名を修正

6bbb4d7

デコレーターにメソッドを移動

6791e22

デコレーターを呼び出すように変更

c10391a

必読判定のテストを追加

b3a16b5

メソッドの返り値をassert/assert_notで判定するよう修正

9588b9f

hikarook94 force-pushed the feature/display-must-read-mark-in-books branch from 086db78 to 9588b9f Compare November 11, 2022 06:56

komagata approved these changes Nov 14, 2022

View reviewed changes

komagata merged commit fbcc104 into main Nov 14, 2022

komagata deleted the feature/display-must-read-mark-in-books branch November 14, 2022 07:00

github-actions bot mentioned this pull request Nov 14, 2022

Release 2022-11-17 08:19:56 #5765

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

参考書籍一覧ページに必読マークを表示する #5674

参考書籍一覧ページに必読マークを表示する #5674

hikarook94 commented Oct 25, 2022 •

edited

Loading

hikarook94 commented Oct 27, 2022

yamatatsu10969 commented Oct 27, 2022

yamatatsu10969 left a comment

yamatatsu10969 Oct 27, 2022

hikarook94 Oct 29, 2022

yamatatsu10969 Oct 30, 2022

hikarook94 Oct 31, 2022

yamatatsu10969 Oct 27, 2022

yamatatsu10969 Oct 27, 2022

hikarook94 Oct 29, 2022

yamatatsu10969 Oct 30, 2022

hikarook94 commented Oct 29, 2022

yamatatsu10969 commented Oct 30, 2022

hikarook94 commented Oct 31, 2022

yamatatsu10969 left a comment

hikarook94 commented Nov 1, 2022

komagata Nov 7, 2022

hikarook94 Nov 7, 2022

komagata Nov 8, 2022

hikarook94 Nov 8, 2022

komagata Nov 11, 2022

hikarook94 Nov 11, 2022

komagata left a comment

		@@ -0,0 +1,7 @@
		# frozen_string_literal: true

		module BooksHelper

		assert_equal true, @book1.must_read_for_any_practices?
		assert_equal false, @book2.must_read_for_any_practices?

参考書籍一覧ページに必読マークを表示する #5674

参考書籍一覧ページに必読マークを表示する #5674

Conversation

hikarook94 commented Oct 25, 2022 • edited Loading

Issue

概要

変更確認方法

変更前

変更後

hikarook94 commented Oct 27, 2022

yamatatsu10969 commented Oct 27, 2022

yamatatsu10969 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hikarook94 commented Oct 29, 2022

yamatatsu10969 commented Oct 30, 2022

hikarook94 commented Oct 31, 2022

yamatatsu10969 left a comment

Choose a reason for hiding this comment

hikarook94 commented Nov 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komagata left a comment

Choose a reason for hiding this comment

hikarook94 commented Oct 25, 2022 •

edited

Loading